-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(tax): add endpoints to manage tax rate rules #6557
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
packages/medusa/src/api-v2/admin/tax-rates/[id]/rules/batch/set/route.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good, few comment and snake case to handle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few comments
* fix * fix * fix: update tax rate with rule setting * fix: add created_by
What
Adds endpoints to manage tax rules on a tax rate:
Noteworthy things I bumped into
Updating nested relationships
A TaxRate can have multiple TaxRules and in this PR we enable users to replace all TaxRules associated with a TaxRate in one operation. If working with the module directly this can be done with:
Internally in the
update
function the TaxModule first soft deletes any TaxRules that exist on the TaxRate and then creates new TaxRules for the passed rules (see test).A challenge arises when doing this in a compensatable way in a workflow. To see this imagine the following:
updateTaxRatesWorkflow
gets the current data for the tax rates to update. This includes the tax rates' rules.updateTaxRatesWorkflow
callstaxModuleService.update
with new rules.As illustrated by 5. compensating the update is not pretty. To get around this I instead opted to let the workflow handle setting the rules for a rate that makes the compensation more straightforward to handle. See workflow here.
Using nested workflows
Initially, I wanted to use the
setTaxRateRulesWorkflow
within theupdateTaxRatesWorkflow
. And this worked great for the invoke phase. However, when I needed to compensate the update workflow (and hence also had to compensate the set rules workflow), I found that the workflow engine no longer had the set rules transaction in memory and therefore could not roll it back. (This is where I try to rollback, but the transaction id can't be found).I therefore opted to copy the steps from the set tax rate rules workflow into the update tax rates workflow; however, once we figure out a good way to ensure we can compensate nested workflows we should move to the nested workflow instead.
This also made me realize that the current implementation of workflows that use
refreshCartPromotions
may create inconsistencies in case of failures (cc: @riqwan).